Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved IndexNotFoundException's default error message #34649

Merged

Conversation

atapin
Copy link
Contributor

@atapin atapin commented Oct 19, 2018

The message is improved by including the missing index name. Fixes #34628.

@s1monw s1monw added the :Core/Infra/Core Core issues without another label label Oct 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left one comment

@@ -37,7 +40,7 @@ public IndexNotFoundException(String index) {
}

public IndexNotFoundException(String index, Throwable cause) {
super("no such index", cause);
super(makeMessage(index), cause);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be ok to duplicate the message like this:

Suggested change
super(makeMessage(index), cause);
super("no such index ["+ index + "]", cause);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also have a convention to embed things like index names in [ ]

@s1monw
Copy link
Contributor

s1monw commented Oct 19, 2018

@elasticmachine ok to test

@atapin atapin force-pushed the 34628-improve-indexnotfoundexception-message branch from 190e09d to 332fdd8 Compare October 22, 2018 22:56
@atapin
Copy link
Contributor Author

atapin commented Oct 23, 2018

Blocked by #34731

@@ -46,7 +46,7 @@ public IndexNotFoundException(Index index) {
}

public IndexNotFoundException(Index index, Throwable cause) {
super("no such index", cause);
super("no such index " + index.toString(), cause);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "no such index [" + index.getName() + "]" as index.toString() could return smth like [index42/n6gzFZTgS664GUfx0Xrpjw] that is a bit inconsistent with other messages

@atapin atapin force-pushed the 34628-improve-indexnotfoundexception-message branch from 332fdd8 to 58ab72c Compare October 23, 2018 13:28
@atapin atapin force-pushed the 34628-improve-indexnotfoundexception-message branch from 58ab72c to 3ab8f53 Compare October 24, 2018 07:45
@vladimirdolzhenko
Copy link
Contributor

vladimirdolzhenko commented Oct 24, 2018

@atapin great job. there are as well yaml-based tests, failed test points to 25_no_auto_create.yml that still expects exception messages w/o index specific.

@atapin
Copy link
Contributor Author

atapin commented Oct 24, 2018

@vladimirdolzhenko oops I missed that guy. thanks for the hint.

@rjernst rjernst merged commit 5f58818 into elastic:master Oct 24, 2018
@rjernst
Copy link
Member

rjernst commented Oct 24, 2018

Thanks for the contribution @atapin!

rjernst pushed a commit to rjernst/elasticsearch that referenced this pull request Oct 24, 2018
This commit adds the index name to the error message when an index is not found.
@atapin atapin deleted the 34628-improve-indexnotfoundexception-message branch October 25, 2018 07:07
kcm pushed a commit that referenced this pull request Oct 30, 2018
This commit adds the index name to the error message when an index is not found.
@rjernst rjernst removed the v6.5.0 label Jan 15, 2019
@rjernst
Copy link
Member

rjernst commented Jan 15, 2019

Note that this was not actually included in 6.5.0 (I have now removed the label). The backport had failures which were difficult to track down, and at this point I do not see a compelling reason to investigate given prereleases of 7.0 are being built.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >enhancement v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants